-
Notifications
You must be signed in to change notification settings - Fork 29.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
path: fix normalize on directories with two dots #14107
Conversation
It's probably not the best way to fix the issue. /cc @mscdex |
test/parallel/test-path.js
Outdated
@@ -406,6 +406,7 @@ assert.strictEqual(path.win32.normalize('a//b//.'), 'a\\b'); | |||
assert.strictEqual(path.win32.normalize('//server/share/dir/file.ext'), | |||
'\\\\server\\share\\dir\\file.ext'); | |||
assert.strictEqual(path.win32.normalize('/a/b/c/../../../x/y/z'), '\\x\\y\\z'); | |||
assert.strictEqual(path.win32.normalize('bar\\foo..\\..\\'), 'bar\\'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add:
assert.strictEqual(path.win32.normalize('bar\\foo..\\..\\'), 'bar\\');
assert.strictEqual(path.win32.normalize('bar\\foo..\\..\\baz'), 'bar\\baz');
assert.strictEqual(path.win32.normalize('bar\\foo..\\'), 'bar\\foo..');
Or alternatively find the coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
eb9ff15
to
1250227
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We could probably refactor this whole thing for TF&I anyway
Good luck. |
This needs a rebase |
1250227
to
092094d
Compare
Looks like I forgot about this PR... Done. |
CI failed on arm, trying again https://ci.nodejs.org/job/node-test-commit-arm/11784/ |
PR-URL: nodejs#14107 Fixes: nodejs#14105 Reviewed-By: Refael Ackermann <[email protected]>
Landed in b98e8d9 |
PR-URL: nodejs/node#14107 Fixes: nodejs/node#14105 Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs/node#14107 Fixes: nodejs/node#14105 Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs#14107 Fixes: nodejs#14105 Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14107 Fixes: #14105 Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14107 Fixes: #14105 Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14107 Fixes: #14105 Reviewed-By: Refael Ackermann <[email protected]>
I've backported to v6.x Please let me know if it should be backed out |
PR-URL: #14107 Fixes: #14105 Reviewed-By: Refael Ackermann <[email protected]>
I've landed this back on v6.x along with 19d2d6611c which fixed the security vulnerability. Please let me know if you think they should be backed out /cc @nodejs/security @nodejs/tsc |
PR-URL: #14107 Fixes: #14105 Reviewed-By: Refael Ackermann <[email protected]>
Fixes: #14105
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
path